Skip to content

Conversation

@juliohm
Copy link
Member

@juliohm juliohm commented Nov 6, 2025

This is a really basic warning, but I think it didn't get fixed because the tests are annoyingly using JuliaFormatter which is not compatible with the latest version of DataStructures.jl?

Could someone please jump in and fix the tests?

@Krastanov
Copy link
Member

The JuliaFormatter test dependency is compat bound to its old version 1.0 because there were problems with 2.0. It seems 1.0 clashes with DataStructures 0.19.

If the problems with 2.0 are fixed, we can rerun the formatter and remove that bound. That is the blocker to making DataStructures 0.19 work.

@Krastanov
Copy link
Member

duplicate of PR #466

@juliohm
Copy link
Member Author

juliohm commented Nov 6, 2025 via email

@Krastanov
Copy link
Member

We can not delete it -- Graphs.jl enforces formatting as a part of its tests. Aggressive quality assurance like this is important for large projects with almost zero maintainers, like Graphs.jl. At least that was the belief of the couple of volunteers that were maintaining this tool in the last couple of years.

Of course, if someone volunteers to be an active maintainer, they would be able to change that policy.

@juliohm
Copy link
Member Author

juliohm commented Nov 6, 2025

I used to be one of the core maintainers of LightGraphs.jl before it was converted into Graphs.jl. Back in the days I don't recall any test that depended on formatting.

Could you please clarify the current situation with a specific test that depends on JuliaFormatter?

@Krastanov
Copy link
Member

formatting attempted again now in #472

@Krastanov
Copy link
Member

I used to be one of the core maintainers of LightGraphs.jl before it was converted into Graphs.jl. Back in the days I don't recall any test that depended on formatting.

Could you please clarify the current situation with a specific test that depends on JuliaFormatter?

Currently I am the only maintainer and I do not have the standing to make changes without input from the community and from more volunteers. I appreciate your dev work from the past, and if you have the bandwidth to get more involved again, that would certainly be appreciated.

The Formatter checks were instituted before I started volunteering to maintain by the previous two caretakers. The argument in favor is that if you do not have enough maintainers you need harsh quality assurance to avoid bitrot. I am weakly in favor of that approach, but will not be unilaterally changing it without other active maintainers on board.

The tests is here:

@testset "Code formatting (JuliaFormatter.jl)" begin

@juliohm
Copy link
Member Author

juliohm commented Nov 6, 2025

I believe this test can be removed in favor of GitHub Actions that check formatting issues. We did set this up across all our currently maintained packages. See Meshes.jl for example, this action: https://github.com/JuliaGeometry/Meshes.jl/blob/master/.github/workflows/FormatPR.yml

Every pull request runs a formatting check and the bot makes suggestions to fix issues. No code is merged otherwise.
The format is specified in this file: https://github.com/JuliaGeometry/Meshes.jl/blob/master/.JuliaFormatter.toml

I appreciate your dev work from the past, and if you have the bandwidth to get more involved again, that would certainly be appreciated.

I can try to help. For some reason I lost write access to Graphs.jl after the conversion from LightGraphs.jl.

@Krastanov
Copy link
Member

added you to the team -- you should have access again

@Krastanov
Copy link
Member

Closing in favor of #472 where the formatting issue is addressed. I cherrypicked the commit from here.

I like the idea of adding the FormatPR.yml workflow, but lets keep the tests too, to avoid accidentally merging things without properly formatted code.

@Krastanov Krastanov closed this Nov 7, 2025
@juliohm
Copy link
Member Author

juliohm commented Nov 7, 2025

I like the idea of adding the FormatPR.yml workflow, but lets keep the tests too, to avoid accidentally merging things without properly formatted code.

The GitHub Action I mentioned will automatically open a PR with fixes related to formatting, so the main branch will always adhere to the specified code style.

I still think that we should not add JuliaFormatter as a test dependency. This is not a dependency that is testing the feature-set of the package, but rather the quality of the code in the repository. For the latter, GitHub Actions are more appropriate and avoid version conflicts like this one we saw that was holding unrelated updates.

Can I implement the GitHub Action for formatting code and remove the format tests to eliminate JuliaFormatter from the test dependencies?

@juliohm juliohm deleted the datastructures-warning branch November 7, 2025 10:45
@Krastanov
Copy link
Member

If no one else complains about this, I consent, as long as the PRs that the formatter action makes are towards the original PR branch, not to master. If the formatter PRs are to master, that would result in a very messy git history with a ton of formatting commits making git blame much harder to use.

The one thing I am strongly opposed to is frequent formatting commits in the master branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants